-
Notifications
You must be signed in to change notification settings - Fork 1.4k
syz-cluster: use base-commit hint from patch series for triage #6557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
If the series author provided a base-commit, use it. If applying the series against the base-commit fails, fall back to the default bisection method.
| continue | ||
| } | ||
| if series.BaseCommitHint == "" { // Usually base-commit is in patch 0 or 1. Check them all to be safe. | ||
| regex := regexp.MustCompile(`(?m)^base-commit:\s*([0-9a-fA-F]{40})$`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't compile the regexp for each message. Ideally it should be done once outside of the function, see the majority of other regexp.MustCompile cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also move this extraction to pkg/email.
| if series.BaseCommitHint == "" { // Usually base-commit is in patch 0 or 1. Check them all to be safe. | ||
| regex := regexp.MustCompile(`(?m)^base-commit:\s*([0-9a-fA-F]{40})$`) | ||
| matches := regex.FindStringSubmatch(email.Body) | ||
| if len(matches) >= 2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just check for nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to avoid an unnecessary crash if the author wrote the tag and forgot the hash. But I just realized it wouldn't match in that case! Will update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be tested.
| SubjectTags []string `json:"subject_tags"` | ||
| PublishedAt time.Time `json:"published_at"` | ||
| Patches []SeriesPatch `json:"patches"` | ||
| BaseCommitHint string `json:"base_commit"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: let's keep json name similar to the field name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rebase it on top of the current master.
|
|
||
| // First try to use hints from the series description. | ||
| if series.BaseCommitHint != "" { | ||
| for _, tree := range trees { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we should be able to determine the tree from the commit:
- Given a commit, we can query the branches that have it.
- Branches are prefixed by tree names (that's the way syz-cluster pulls them).
You can use #6553 as reference here.
If the series author provided a base-commit, use it. If applying the series against the base-commit fails, fall back to the default bisection method.
Before sending a pull request, please review Contribution Guidelines:
https://github.com/google/syzkaller/blob/master/docs/contributing.md